Skip to content

Rewrite ReactPy-Router #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 62 commits into from
Oct 14, 2024
Merged

Conversation

Archmonger
Copy link
Contributor

@Archmonger Archmonger commented Jun 22, 2024

Description

Fixes a boatload of rendering bugs and aligns the API with react router.

Changed

  • Bump GitHub workflows
  • Rename use_query to use_search_params.
  • Rename simple.router to browser_router.
  • Rename SimpleResolver to StarletteResolver.
  • Rename CONVERSION_TYPES to CONVERTERS.
  • Change "Match Any" syntax from a star * to {name:any}.
  • Rewrite reactpy_router.link to be a server-side component.
  • Simplified top-level exports within reactpy_router.

Added

  • New error for ReactPy router elements being used outside router context.
  • Configurable/inheritable Resolver base class.
  • Add debug log message for when there are no router matches.
  • Add slug as a supported type.

Fixed

  • Fix bug where changing routes could cause render failure due to key identity.
  • Fix bug where "Match Any" pattern wouldn't work when used in complex or nested paths.
  • Fix bug where link elements could not have @component type children.
  • Fix bug where the ReactPy would not detect the current URL after a reconnection.
  • Fixed flakey tests on GitHub CI by adding click delays.

Checklist

Please update this checklist as you complete each item:

  • Tests have been developed for bug fixes or new functionality.
  • The changelog has been updated, if necessary.
  • Documentation has been updated, if necessary.
  • GitHub Issues closed by this PR have been linked.

By submitting this pull request I agree that all contributions comply with this project's open source license(s).

This was linked to issues Jun 25, 2024
@Archmonger
Copy link
Contributor Author

While the tests may have passed on this PR, the tests are still extremely flakey.

The tests being flakey appears to already be a problem on main, and is probably related to reactpy.testing.DisplayFixture.

Will need a follow up PR to resolve this.

@Archmonger Archmonger marked this pull request as ready for review October 11, 2024 10:55
@Archmonger Archmonger requested a review from a team as a code owner October 11, 2024 10:55
@rmorshea
Copy link
Contributor

Will take a look this weekend.

@Archmonger
Copy link
Contributor Author

Archmonger commented Oct 11, 2024

Fixed the flakey tests by adding some click delays.

I saw this same problem on reactpy-django, where the GitHub CI lags enough to where you can click an element before reactpy finishes re-rendering, which causes a click event to be sent to a non-existent element.

This is technically a problem with ReactPy core, but I don't see a way we could fix this.

As a note, this is probably the same reason why tests are flakey on core's CI as well.

@Archmonger Archmonger linked an issue Oct 12, 2024 that may be closed by this pull request
@Archmonger Archmonger removed a link to an issue Oct 12, 2024
Comment on lines +61 to +92
# FIXME: This component currently works in a "dumb" way by trusting that ReactPy's script tag \
# properly sets the location due to bugs in ReactPy rendering.
# https://github.com/reactive-python/reactpy/pull/1224
current_path = use_connection().location.pathname

@event(prevent_default=True)
def on_click(_event: dict[str, Any]) -> None:
pathname, search = to.split("?", 1) if "?" in to else (to, "")
if search:
search = f"?{search}"

# Resolve relative paths that match `../foo`
if pathname.startswith("../"):
pathname = urljoin(current_path, pathname)

# Resolve relative paths that match `foo`
if not pathname.startswith("/"):
pathname = urljoin(current_path, pathname)

# Resolve relative paths that match `/foo/../bar`
while "/../" in pathname:
part_1, part_2 = pathname.split("/../", 1)
pathname = urljoin(f"{part_1}/", f"../{part_2}")

# Resolve relative paths that match `foo/./bar`
pathname = pathname.replace("/./", "/")

set_location(Location(pathname, search))

attrs["onClick"] = on_click

return html._(html.a(attrs, *children), html.script(link_js_content.replace("UUID", uuid_string)))
Copy link
Contributor Author

@Archmonger Archmonger Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this solution isn't ideal, but I'd rather let this PR move forward with this short-term solution so I can continue tackling the other issues in reactpy-router.

I'll need to do some fixes on ReactPy core to allow me to uncomment the client-side approach from def link().

@Archmonger Archmonger merged commit 0fe17fb into reactive-python:main Oct 14, 2024
8 checks passed
@Archmonger Archmonger deleted the v1-rewrite branch October 14, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links only work with string arguments Rename router to browser_router
2 participants